Repository navigation
refactor: simplify Node installation guidance - #60
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| "Keep an existing writable npm prefix only if its bin is separate from Python's scripts directory. " | ||
| "If its bin is missing from PATH, add it before /usr/bin " | ||
| "and after your Python scripts (for example /opt/venv/bin or /usr/local/bin).", |
| "If an inherited NPM_CONFIG_PREFIX points to an unwritable directory, change it in the Dockerfile " | ||
| "(for example `ENV NPM_CONFIG_PREFIX=/home/app/.npm-global`). Without that override, a non-root user " | ||
| "can run `npm config set prefix ~/.npm-global`. Add the writable bin to PATH in either case.", |
| " RUN dnf --releasever=latest install -y nodejs24 nodejs24-npm " | ||
| "&& /usr/sbin/alternatives --set node /usr/bin/node-24 && dnf clean all", |
| "Node 24 requires repository release 2023.9.20251110 or newer; replace latest with your approved recent " | ||
| "snapshot, or update an older base image before installing.", |
| "If your pinned repository is older than 2023.9.20251110, update the base/repository or install from a " | ||
| "newer approved snapshot: sudo dnf --releasever=latest install -y nodejs24 nodejs24-npm", |
| "If you choose a container image instead, create a new image-based function " | ||
| "and install Node at build time.", |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Serverless guidance still includes misleading runtime installation instructions.
Review effort: Lite
Findings: None
What changed in this PR
Refactors Node.js installation guidance to be shorter and deployment-aware across platforms, containers, serverless environments, CI, and Windows.
Changes:
- Simplifies environment detection and installation instructions.
- Adds Kubernetes, Docker, distribution-specific, serverless, and Windows guidance.
- Expands Docker and platform validation tests.
- Replaces the duplicate release FAQ with the versioning policy link.
| File | Description |
|---|---|
tests/test_instructions.py |
Updated instruction coverage. |
tests/test_environment.py |
Updated environment detection tests. |
tests/smoke/test_installation.py |
Added Windows command smoke tests. |
src/promptfoo/instructions.py |
Generates contextual Node.js guidance. |
src/promptfoo/environment.py |
Simplifies environment detection. |
AGENTS.md |
Consolidates release FAQ guidance. |
.github/workflows/test.yml |
Validates generated Dockerfiles. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
mldangelo
left a comment
mldangelo
left a comment
There was a problem hiding this comment.
Reviewed platform detection and installation guidance, including source-only serverless handling, Python image preservation, writable npm prefixes, and Windows launchers. Fixed the non-root Docker fixture install and updated the Windows help expectation. Local Ruff, mypy, pyright, actionlint, 195 unit tests, 20 installed-wrapper smoke tests, missing-Node CLI help, and wheel/sdist builds pass. All 18 current-head GitHub checks pass, including Docker and Windows validation.
The missing-Node help now uses the detected platform and deployment context to show a shorter installation guide. It preserves the existing Python image for Debian and Ubuntu containers, accounts for Amazon Linux compatibility and serverless hosting restrictions, and distinguishes hosted functions from local development. Kubernetes receives container guidance.
Windows verification and direct usage use
npx.cmd, with annpx.exealternative for compatible Node managers. The Docker check installs a packed fixture so a non-root npm prefix does not link to root-owned source files. The duplicate release FAQ now links to the existing versioning policy.Validation: Ruff, mypy, pyright, actionlint, 195 unit tests, 20 installed-wrapper npx smoke tests, the actual missing-Node CLI path, and wheel/sdist builds pass locally. Four Windows smoke cases are platform-skipped locally. Fresh PR CI validates the five Linux container configurations, Windows PowerShell/Git Bash commands, and the Python/Node global-install and npx-fallback matrices. A local Docker daemon and native Windows are unavailable.